[SM-999] Add Bulk Move to Project Endpoint #66

lizard-boy · 2024-10-19T09:04:29Z

Type of change

- [ ] Bug fix
- [x] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Add new endpoint for updating the projects of many secrets to the same project.

Clients PR: bitwarden/clients#6665

Code changes

bitwarden_license/src/Commercial.Infrastructure.EntityFramework/SecretsManager/Repositories/SecretRepository.cs: This interacts with the virtual table ProjectSecret to avoid making large changes in the code base or to make this update one row at a time. It deletes all current relationships for the given secrets and then creates new relationships for them based on the supplied project ids.
src/Api/SecretsManager/Controllers/SecretsController.cs: Controller method for validating that the current user has the permissions to do the specified action.

Before you submit

Please check for formatting errors (dotnet format --verify-no-changes) (required)
If making database changes - make sure you also update Entity Framework queries and/or migrations
Please add unit tests where it makes sense to do so (encouraged but not required)
If this change requires a documentation update - notify the documentation team
If this change has particular deployment requirements - notify the DevOps team

Greptile Summary

This pull request adds a new endpoint for bulk moving secrets to a project in the Secrets Manager, including necessary authorization, command implementation, and unit tests.

Added BulkMoveToProjectAsync method in SecretsController.cs to handle the new bulk move endpoint
Implemented MoveSecretsCommand and BulkSecretAuthorizationHandler for executing and authorizing bulk secret operations
Created ProjectSecret entity to represent many-to-many relationships between projects and secrets
Added new methods AccessToSecretsAsync and MoveSecretsAsync to ISecretRepository interface and implementations
Created empty migration files for MySQL, PostgreSQL, and SQLite that need to be properly implemented

…roject

- Use EntityType for Join Table

Co-authored-by: Thomas Avery <[email protected]>

…roject

greptile-apps

_{26 file(s) reviewed, 24 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

greptile-apps · 2024-10-19T09:04:57Z

...mmercial.Core/SecretsManager/AuthorizationHandlers/Secrets/BulkSecretAuthorizationHandler.cs

+      BulkSecretOperationRequirement requirement,
+      IReadOnlyList<Secret> resource)
+    {
+        var secretsByOrganizationId = resource.GroupBy(s => s.OrganizationId).ToArray();


style: Consider using FirstOrDefault() instead of ToArray() for better performance when only checking for a single group.

greptile-apps · 2024-10-19T09:04:58Z

...mmercial.Core/SecretsManager/AuthorizationHandlers/Secrets/BulkSecretAuthorizationHandler.cs

+        var secretAccesses = await _secretRepository.AccessToSecretsAsync(
+          secrets.Select(s => s.Id).ToArray(), userId, accessClientType);


style: This could potentially be a performance bottleneck for large numbers of secrets. Consider implementing a batch operation in the repository.

greptile-apps · 2024-10-19T09:04:58Z

...mmercial.Core/SecretsManager/AuthorizationHandlers/Secrets/BulkSecretAuthorizationHandler.cs

+          secrets.Select(s => s.Id).ToArray(), userId, accessClientType);
+
+        // If we don't have the write permission
+        return secretAccesses.All(a => a.Value.Write);


logic: Ensure that secretAccesses contains an entry for every secret, otherwise this check might pass incorrectly.

greptile-apps · 2024-10-19T09:06:04Z

...rc/Commercial.Infrastructure.EntityFramework/SecretsManager/Repositories/SecretRepository.cs

+        await dbContext.ProjectSecrets
+            .Where(ps => secretIds.Contains(ps.SecretsId))
+            .ExecuteDeleteAsync();


logic: This operation deletes all existing project-secret relationships. Ensure this is the intended behavior, as it may have unintended consequences

greptile-apps · 2024-10-19T09:06:05Z

...rc/Commercial.Infrastructure.EntityFramework/SecretsManager/Repositories/SecretRepository.cs

+        Guid userId,
+        AccessClientType accessType)
+    {
+        await using var scope = ServiceScopeFactory.CreateAsyncScope();


style: Use 'using' instead of 'await using' for consistency with other methods

greptile-apps · 2024-10-19T09:13:01Z

util/PostgresMigrations/Migrations/20240127135659_CreateProjectSecretEntity.Designer.cs

+            modelBuilder.Entity("Bit.Infrastructure.EntityFramework.SecretsManager.Models.ProjectSecret", b =>
+                {
+                    b.HasOne("Bit.Infrastructure.EntityFramework.SecretsManager.Models.Project", null)
+                        .WithMany()
+                        .HasForeignKey("ProjectsId")
+                        .OnDelete(DeleteBehavior.Cascade)
+                        .IsRequired();
+
+                    b.HasOne("Bit.Infrastructure.EntityFramework.SecretsManager.Models.Secret", null)
+                        .WithMany()
+                        .HasForeignKey("SecretsId")
+                        .OnDelete(DeleteBehavior.Cascade)
+                        .IsRequired();
+                });


logic: Cascade delete behavior for ProjectSecret relationships may cause unintended data loss if not carefully managed

greptile-apps · 2024-10-19T09:13:22Z

util/PostgresMigrations/Migrations/20240127135659_CreateProjectSecretEntity.cs

+    protected override void Up(MigrationBuilder migrationBuilder)
+    {
+
+    }


logic: Up method is empty. Implement table creation, indexes, and foreign keys for ProjectSecret entity.

greptile-apps · 2024-10-19T09:13:23Z

util/PostgresMigrations/Migrations/20240127135659_CreateProjectSecretEntity.cs

+    protected override void Down(MigrationBuilder migrationBuilder)
+    {
+
+    }


logic: Down method is empty. Implement logic to revert changes made in Up method.

greptile-apps · 2024-10-19T09:14:41Z

util/SqliteMigrations/Migrations/20240127135708_CreateProjectSecretEntity.cs

+    protected override void Up(MigrationBuilder migrationBuilder)
+    {
+
+    }


logic: The Up method is empty. It should create the ProjectSecret table with appropriate columns.

greptile-apps · 2024-10-19T09:14:41Z

util/SqliteMigrations/Migrations/20240127135708_CreateProjectSecretEntity.cs

+    protected override void Down(MigrationBuilder migrationBuilder)
+    {
+
+    }


logic: The Down method is empty. It should drop the ProjectSecret table to revert the migration.

justindbaur and others added 19 commits October 23, 2023 07:22

Add Bulk Move to Project Endpoint

4c4b5ea

Formatting

2a9b576

Merge remote-tracking branch 'origin/main' into sm-add-bulk-move-to-p…

7632a3c

…roject

Make Project Singular

d4217c6

- Use EntityType for Join Table

Update src/Api/SecretsManager/Controllers/SecretsController.cs

80f7446

Co-authored-by: Thomas Avery <[email protected]>

Add ProjectSecret as real entity

bc2c17d

Use Authorization Handler

7dae83e

Write Tests

c66865b

Rename Operation

f7a8bd6

Lint

13b8de6

Remove Accidental Change

19a0889

Merge remote-tracking branch 'origin/main' into sm-add-bulk-move-to-p…

1462cff

…roject

Address PR Feedback

f92532c

Move Logic In Controller

46418d9

Format

a4a984c

Refresh Migrations

b38b87c

Format

4612829

Cover more flows

d0cb590

Formatting

3a9b06e

greptile-apps bot reviewed Oct 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SM-999] Add Bulk Move to Project Endpoint #66

[SM-999] Add Bulk Move to Project Endpoint #66

lizard-boy commented Oct 19, 2024 •

edited by greptile-apps bot

Loading

greptile-apps bot left a comment

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

greptile-apps bot Oct 19, 2024

		var secretAccesses = await _secretRepository.AccessToSecretsAsync(
		secrets.Select(s => s.Id).ToArray(), userId, accessClientType);

[SM-999] Add Bulk Move to Project Endpoint #66

Are you sure you want to change the base?

[SM-999] Add Bulk Move to Project Endpoint #66

Conversation

lizard-boy commented Oct 19, 2024 • edited by greptile-apps bot Loading

Type of change

Objective

Code changes

Before you submit

Greptile Summary

greptile-apps bot left a comment

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Oct 19, 2024

Choose a reason for hiding this comment

lizard-boy commented Oct 19, 2024 •

edited by greptile-apps bot

Loading